Skip to content

Autotools: Add pkg-config for GMP library #15166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Aug 6, 2024
Merged

Autotools: Add pkg-config for GMP library #15166

merged 2 commits into from
Aug 6, 2024

Conversation

petk
Copy link
Member

@petk petk commented Jul 30, 2024

This can also wait for the next PHP version or perhaps for PHP 8.4 beta otherwise it seems to be working fine.

GMP has pkg-config integration since 2019-08-22 (version ~6.2.0).

This optionally finds the GMP library using pkg-config or falls back to find library on the system or with the provided configure option argument (--with-gmp=DIR).

When using DIR argument, the pkg-config check is silently skipped.

When not using DIR argument, the GMP_CFLAGS and GMP_LIBS can be also used to find the GMP library:

 ./configure --with-gmp \
     GMP_CFLAGS=-I/path/to/gmp/include \
     GMP_LIBS="-L/path/to/gmp -lgmp" \

@petk petk requested a review from NattyNarwhal July 30, 2024 09:40
@petk petk requested a review from Girgias as a code owner July 30, 2024 09:40
devnexen
devnexen approved these changes Jul 30, 2024
View reviewed changes
Copy link
Member

@devnexen devnexen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MSTM

@NattyNarwhal
Copy link
Member

Could omit the =dir handling and just rely on the PKG_CHECK_MODULES override behaviour for i.e. GMP_CFLAGS?

Girgias
Girgias reviewed Jul 30, 2024
View reviewed changes
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, and this can land in the beta IMHO

@petk
Copy link
Member Author

petk commented Jul 30, 2024

Could omit the =dir handling and just rely on the PKG_CHECK_MODULES override behaviour for i.e. GMP_CFLAGS?

Yes, I think removing the DIR argument could be possible and not problematic. In the current implementation (as is in the current master branch, not in this PR) this DIR is not working ok on many *nix installations because gmp is actually installed to a compiler default locations (such as machine includes, library path of /usr/lib/x86_64_linux-gnu etc.) So, if pkg-config doesn't find it it falls back to just checking the default paths (like it does already) and if still not found some sensible error is thrown. I'll recheck and test this a bit to not miss anything.

@petk
Copy link
Member Author

petk commented Aug 5, 2024

Issue with removing the DIR argument --with-gmp=DIR is that it is used quite a lot probably. Even in PHP CI files:

php-src/.github/actions/configure-macos/action.yml

Line 55 in 1251901

--with-gmp="$BREW_OPT"/gmp \

So, I would suggest to go with using optional pkg-config and then resorting to system paths as a fallback. And optionally check the DIR argument here.

petk added 2 commits August 5, 2024 09:31
@petk
Autotools: Add pkg-config for GMP library
7996604 
GMP has pkg-config integration since 2019-08-22 (version ~6.2.0).

This optionally finds the GMP library using pkg-config or falls back to
find library on the system or with the provided configure option
argument (--with-gmp=DIR).

When using DIR argument, the pkg-config check is silently skipped.

When not using DIR argument, the GMP_CFLAGS and GMP_LIBS can be also
used to find the GMP library:

        ./configure --with-gmp \
            GMP_CFLAGS=-I/path/to/gmp/include \
            GMP_LIBS="-L/path/to/gmp -lgmp"
@petk
Fix nits and add note to UPGRADING.INTERNALS
a5bcd0c
@petk petk force-pushed the patch-gmp-pkgconf branch from cbc37fe to a5bcd0c Compare August 5, 2024 07:34
NattyNarwhal
NattyNarwhal approved these changes Aug 5, 2024
View reviewed changes
Copy link
Member

@NattyNarwhal NattyNarwhal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel we could tell people to set the manual overrides or adjust PKG_CONFIG_PATH, but this is also fine as it stands.

@petk petk merged commit 0c3b767 into php:master Aug 6, 2024
10 of 11 checks passed
@petk petk deleted the patch-gmp-pkgconf branch August 6, 2024 08:16
@petk
Copy link
Member Author

petk commented Aug 6, 2024

I feel we could tell people to set the manual overrides or adjust PKG_CONFIG_PATH, but this is also fine as it stands.

Yes, PHP started adopting this pkg-config at some point very heavily. But passing the library installation directory isn't something bad or old. If the library follows some minimum standards and has GNU-like installation directory structure it is actually simpler to use than adjusting pkg-config variables.

Also, just for example, CMake has PackageName_ROOT variables that are somehow similar to these DIR arguments. They of course would like to see CMake configuration exported by the library itself, so that find_package(PackageName) would work out of the box without additional adjustment by the developer using the PackageName but in practice this turned out that most libraries don't follow this. Or if the library chooses Meson build system, then build system specific wishes can't work. Probably the pkg-config should be the defacto standard in C for finding these packages but it is moving very slowly and isn't something very reliable. It is quite difficult to master these build system specific stuff for some library maintainer. Not to mention that pkg-config isn't available on Windows. :/ Yes, C package management is very problematic :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Girgias Girgias Girgias left review comments

@NattyNarwhal NattyNarwhal NattyNarwhal approved these changes

@devnexen devnexen devnexen approved these changes

Assignees
No one assigned
Labels
Category: Build System Extension: gmp
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@petk @NattyNarwhal @devnexen @Girgias